Refactor lists::contains#11019
Merged
rapids-bot[bot] merged 218 commits intorapidsai:branch-22.08from Jun 23, 2022
Merged
Conversation
ttnghia
requested review from
jrhemstad
and removed request for
codereport and
vyasr
rapids-bot bot
pushed a commit
that referenced
this pull request
Jun 16, 2022
This PR updates `clamp.cu` to take templated iterator types by value. This aligns the style with the rest of libcudf. See also: #11019 (comment) Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Nghia Truong (https://github.com/ttnghia) - David Wendt (https://github.com/davidwendt) - Mark Harris (https://github.com/harrism) URL: #11084
jrhemstad
reviewed
Jun 16, 2022
jrhemstad
reviewed
Jun 16, 2022
jrhemstad
reviewed
Jun 16, 2022
jrhemstad
reviewed
Jun 16, 2022
bdice
approved these changes
Jun 22, 2022
Contributor
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
Nice. This has improved a lot with review.
Contributor
Author
|
Thanks all for reviewing. Yeah, the implementation has been improved significantly with your help ❤️ |
# Conflicts: # cpp/src/lists/contains.cu
karthikeyann
approved these changes
Jun 23, 2022
Contributor
karthikeyann
left a comment
karthikeyann
left a comment
There was a problem hiding this comment.
Minor comments. Rest look good. 👍
karthikeyann
approved these changes
Jun 23, 2022
Contributor
Author
|
@gpucibot merge |
rapids-bot bot
pushed a commit
that referenced
this pull request
Aug 22, 2022
This extends the `lists::contains` API to support nested types (lists + structs) with arbitrarily nested levels. As such, `lists::contains` will work with literally any type of input data. In addition, the related implementation has been significantly refactored to facilitate adding new implementation. Closes #8958. Depends on: * #10730 * #10883 * #10999 * #11019 * #11037 Authors: - Nghia Truong (https://github.com/ttnghia) - Bradley Dice (https://github.com/bdice) Approvers: - MithunR (https://github.com/mythrocks) - Bradley Dice (https://github.com/bdice) URL: #10548
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is just a not-very-simple refactor to
lists::contains:lists/contains.hpp, and rewrite doxygen there a little bit.No new feature is added in this PR, just modifying the existing functions and moving things around.
This PR is extracted from the bigger PR for easier review. The original PR is #10548 for supporting nested type in
lists::contains. As such, this blocks it.